Thanks for looking into this!

@yury-fedotov Thanks for reporting the issue :)

Thanks! I overlooked this. Can we keep the change at the same line? This is changing the order of arguments, is it needed?

@noklam It was erroring out saying arguments with default values cannot be before ones without default values

Thanks! I overlooked this. Can we keep the change at the same line? This is changing the order of arguments, is it needed?

@noklam It was erroring out saying arguments with default values cannot be before ones without default values

Unfortunately, changing the order of arguments is a breaking change. So in this case it's better to ignore the error about argument order instead.

I've gone back to the original order but added default values to subsequent arguments. Not sure if this has any implications but the tests seem to be passing. The other way to fix the issue linked would be to change the example test in starters. WDYT? 🤔 @noklam @merelcht

    env: str | None = field(init=True, default=None)
    _package_name: str = field(init=True, default="")
    _hook_manager: PluginManager | None = field(init=True, default=None)

Unfortunately I think there is no nice solution here. ^ I don't think there should be a default for hook_manager, does it even work when it is None?

For package_name, I did a quick search and find no reference, I don't really know why it is needed. The problem is we cannot add a default for env without also adding default for all subsequent arguments. If we add default to all of them it's also a breaking change. (adding only to env is OK because I consider that's a bug fix, but it's not possible as mentioned).

My suggestion is not to fix anything in kedro, but remove the test. In fact, I don't think the KedroContext has any use in tests. In a standard kedro run, we should create the session and let the session to create context, runner, config_loader etc. However, in our test template, we created the config loader and runner manually. In addition, the context actually use the real hooks, but the tests that created run with no hook, so they are not consistent at all.

In our own testing docs, KedroContext is not mentioned at all.

@noklam yeah, i'll add your comment to the issue and put that in backlog and close this PR for now!